Skip to content

Conversation

@BalkanMadman
Copy link

Hello!

We've been packaging QuickJS-ng in Gentoo and we've found meson to be the most versatile amongst the added build systems. Huge thanks for taking care of QuickJS and implementing modern build systems -- it makes the life of us, packagers, much much easier.

This PR bundles two simple fixes for meson.build

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 4, 2025

As far as I'm aware, the CI failures are not caused by these changes

@BalkanMadman
Copy link
Author

Ping. Any updates?

@saghul
Copy link
Contributor

saghul commented Nov 18, 2025

I've triggered the CI again. IIRC the failures were directly related to this change, but let's see.

@bnoordhuis
Copy link
Contributor

bnoordhuis commented Nov 18, 2025

I'm fine with this change. @saghul?

edit: cross-wired

@BalkanMadman
Copy link
Author

I've been going through CI and stumbled upon this: https://github.com/quickjs-ng/quickjs/actions/runs/19475324220/job/55744866380?pr=1225#step:7:42

meson.build:624: WARNING: Trying to use ['address', 'undefined'] sanitizer on Clang with b_lundef.

@BalkanMadman
Copy link
Author

I can reproduce the failure on my machine too (albeit with clang only).

@BalkanMadman
Copy link
Author

https://github.com/google/sanitizers/wiki/AddressSanitizer#faq:

Q: When I link my shared library with -fsanitize=address, it fails due to some undefined ASan symbols (e.g. asan_init_v4)?

A: Most probably you link with -Wl,-z,defs or -Wl,--no-undefined. These flags don't work with ASan unless you also use -shared-libasan (which is the default mode for GCC, but not for Clang).

@BalkanMadman
Copy link
Author

Meson option b_lundef defaults to true, e.g. -Wl,--no-undefined is added to CFLAGS.

@BalkanMadman
Copy link
Author

Passing -shared-libasan seems to fix the error.

@BalkanMadman
Copy link
Author

Should fix the CI.

@saghul
Copy link
Contributor

saghul commented Nov 19, 2025

Yeah so as I said, the meson CI needs updating now to statically link by default.

@BalkanMadman
Copy link
Author

Would you like to me to do the CI update (including the fix for the failing CI for this PR) in this PR or separately?

@saghul
Copy link
Contributor

saghul commented Nov 21, 2025

Would you like to me to do the CI update (including the fix for the failing CI for this PR) in this PR or separately?

In this PR please, we like to avoid merging failing PRs.

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 22, 2025

Would you like to me to do the CI update (including the fix for the failing CI for this PR) in this PR or separately?

In this PR please, we like to avoid merging failing PRs.

Your release CI builds QuickJS-NG with CMake so this PR shouldn't affect that. The PR already contains a fix for PR CI. My apologies, screwed up the quoting

@BalkanMadman
Copy link
Author

Apparently, for MSVC CI, we're hitting mesonbuild/meson#13892. Will add a commit forcing MSVC-based builds to use static libs.

@bnoordhuis
Copy link
Contributor

@BalkanMadman ping, seems this was close to done?

@BalkanMadman
Copy link
Author

Hello! Sorry for the delay, could you please try running the CI again? The two new commits should fix two issues in building QuickJS-NG shared.

@BalkanMadman BalkanMadman force-pushed the meson-fixes branch 2 times, most recently from 5b8d06a to 0cf1f7f Compare December 24, 2025 08:54
@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 24, 2025

Okay, I also need to guard all the dll{export,import} shenanigans if we're building static libqjs🔥🔥🔥

Because obviously just exporting symbols isn't enough and on Windows you need separate exports for DLLs and static libs👍

@saghul
Copy link
Contributor

saghul commented Dec 24, 2025

Not sure I get the unconditional define in quickjs.c. Shouldn't it be set by Meson depending on the build type?

@saghul
Copy link
Contributor

saghul commented Dec 26, 2025

Looks like there are conflicts, can you rebase?

@BalkanMadman BalkanMadman requested a review from saghul January 5, 2026 14:46
@BalkanMadman BalkanMadman changed the title meson.build fixes CMake/meson fixes, Windows support for shared lib build Jan 5, 2026
#endif /* QUICKJS_NG_PLAT_WIN32 */

/*
* `JS_LIBC_EXTERN` -- helper macro that must be used to mark the extern
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a problem if we don't use this and simply use JS_EXTERN directly in the libc?

Copy link
Author

@BalkanMadman BalkanMadman Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do that when libc isn't built as a part of shared qjs.dll on Windows.

In this case, in quickjs-libc, we need JS_LIBC_EXTERN to be empty, while JS_EXTERN is __declspec(dllexport) (for qjs) or __declspec(dllimport) (for qjs consumers). Both break libc's builds on Windows.

For all other cases, it doesn't make much sense to duplicate all the logic.

Copy link
Author

@BalkanMadman BalkanMadman Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean, is that we need to have some additional logic for specifically the libc version of JS_EXTERN in any case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, to me, overloading JS_EXTERN for that doesn't seem to be the most ergonomic solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I fully follow. Both when built as static and shared library the libc is part of the core library build so it should follow the same rules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy: #1312

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apropos the dllimport/dllexport dichotomy, in libuv we solve that like this:

https://github.com/libuv/libuv/blob/91ae02a63d25caccddd835fb201195fefe9d113b/include/uv.h#L34-L46

In a nutshell: it's up to the first party/third party build script to set BUILDING_UV_SHARED or USING_UV_SHARED.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy: #1312

Oh, I didn't actually bother to check whether the ptrs are used in one place or all over the libc.

Copy link
Author

@BalkanMadman BalkanMadman Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apropos the dllimport/dllexport dichotomy, in libuv we solve that like this:

https://github.com/libuv/libuv/blob/91ae02a63d25caccddd835fb201195fefe9d113b/include/uv.h#L34-L46

In a nutshell: it's up to the first party/third party build script to set BUILDING_UV_SHARED or USING_UV_SHARED.

That's what's done in this PR, albeit in a bit different way (inspired by zlib). In contrast to libuv, I added one macro for shared qjs (either building or using) — QUICKJS_NG_DLL — and one macro for when qjs is built: QUICJS_NG_QJS_INTERNAL.

I think that as a user definable macro, QUICKJS_NJ_DLL sounds better than USING_SHARED_QJS but that's just my style preference and I can change the macro names.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on this?

Functions declared by cutils.h are used by most of the source files in
the project. However, they are not part of libqjs API and, thus, the
functions are not marked as API.

Apart from the fact that cutils do not belong in libqjs, since they do
not constitute a public API and are just helpers, they break shared
qjs.dll builds on Windows where DLLs must explicitly mark their
interfaces.

As cutils.h does not mark its functions with JS_EXTERN or alike, the
Windows linker is unable to resolve references to the cutils functions.

This commit makes cutils a separate static library, which is linked with
the dependent executables/libraries.

Signed-off-by: Zurab Kvachadze <[email protected]>
Previously, irrespective of the setting of -Dlibc, quickjs-libc would be
built as a static library. In case -Dlibc=true is set, the static
library is linked in libqjs only. In case -Dlibc=false is set, the
archive is linked to every quickjs-libc consumer.

In the former case (-Dlibc=true), building quickjs-libc as static
library introduces useless indirection, since quickjs-libc is going to
be linked only to libqjs anyway. CMake just adds the libc's sources to
qjs's and calls it a day.

This commit changes meson.build. If -Dlibc=true is set, quickjs-libc's
sources are compiled as a part of libqjs. If not, it compiled as a
static library, as was done before.

In both cases, a direct dependency of quickjs-libc has been changed into
a `dependency` on qjs_libc_dep which conveniently abstracts both
configuration.

So, if quickjs-libc needs to be linked to, just add `dependencies:
qjs_libc_dep`.

Signed-off-by: Zurab Kvachadze <[email protected]>
Previously, if QJS_BUILD_LIBC was false, quickjs-libc would be built
with every consumer. This made it harder to specify and trace the
dependencies/compiler definitions of quickjs-libc itself.

With this change, if -DQJS_BUILD_LIBC=false is passed to CMake,
quickjs-libc is built as a normal static library, with its own compiler
definitions and dependencies.

This changes CMake to match the Meson behaviour. Additionally, since
CMake does not allow mixing keyword and non-keyword declarations, this
commit adds explicit `PRIVATE` to all `target_link_libraries`
declarations.

Signed-off-by: Zurab Kvachadze <[email protected]>
Since the macros like JS_EXTERN are used in more than one place, it is
quite reasonable and helpful to declare them in one place. Additionally,
the Windows support with the __declspec tango was missing.

This commit, in addition to reorganising definitions, also plumbs
Windows support for the renamed QJS_EXTERN, QJS_LIBC_EXTERN and
QJS_FORCE_INLINE.

Signed-off-by: Zurab Kvachadze <[email protected]>
@BalkanMadman
Copy link
Author

Rebased, should be mergeable now.

Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd like @bnoordhuis to also take a look. Nice work!

@BalkanMadman
Copy link
Author

The CI failures are because examples/fib.c piggybacks on JS_EXTERN from quickjs.h:

https://github.com/BalkanMadman/quickjs/blob/ecc73f14545fb5723d1531e265f97adf7273803f/examples/fib.c#L64

JS_EXTERN is defined to __declspec(dllimport) because the logic (correctly) concludes that fib.c is a consumer of quickjs.h APIs.

The right way to do it is to define a helper macro like JS_MODULE_EXTERN which properly expands to whatever we need for module, i.e. __attribute__((visibility("default))) or __declspec(dllexport). I don't whether it's better to define this macro in quickjs.h (so that the macro doesn't need to be defined in each module) or in each individual module.

What do you think @saghul @bnoordhuis?

@saghul
Copy link
Contributor

saghul commented Jan 15, 2026

JS_MODULE_EXTERN

Sounds good to me!

@BalkanMadman
Copy link
Author

Would you like that to be defined in quickjs.h or in each module?

@saghul
Copy link
Contributor

saghul commented Jan 15, 2026

Would you like that to be defined in quickjs.h or in each module?

I think it would be useful to define it in quickjs.h so that modules can reuse it.

@BalkanMadman
Copy link
Author

Meson doesn't set -DJS_SHARED_LIBRARY for modules in examples/. Should that be fixed too? Also, what is the purpose of this define?

@BalkanMadman
Copy link
Author

It was seemingly introduced in fib.c in 91459fb. I don't see a point in it though? Should it just be removed?

@saghul
Copy link
Contributor

saghul commented Jan 15, 2026

If it's not used then it can go yeah.

@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 16, 2026

The new commit: e43bb25. The rest of the commits are unchanged. Tested test_fib.js with CMake and meson on Linux, binary modules load and work correctly.

Will now test Windows.

The new macro is a counterpart to JS_EXTERN, but for binary C modules.
This commit introduces one more preprocessor define that must be set
when building C modules.

This commit changes examples/fib.c and examples/point.c to use the new
macro, while also removing redundant JS_SHARED_LIBRARY. Additionally,
the includes use the angled brackets now since quickjs.h is supposed to
be external to the binary modules.

meson.build is fixed to properly add the include directory; the manual
header copying has also been removed as it is redundant.

Signed-off-by: Zurab Kvachadze <[email protected]>
Clang cannot handle building shared libraries with sanitizers and
-Wl,--no-undefined (set by default unless explicitly disabled with
-Db_lundef=false).

This commit prefixes CI in case shared libraries are built with
sanitisers.

Signed-off-by: Zurab Kvachadze <[email protected]>
During the build, the default library can be overridden via the
-Ddefault_library=type flag.

Presetting this key in meson.build makes life harder for distributions
which almost always want to build shared libraries. Those requiring
static libraries can always force that via the aforementioned flag.

Signed-off-by: Zurab Kvachadze <[email protected]>
@BalkanMadman
Copy link
Author

The CI should do better now. Tested on Windows with the just applied changes: everything works.

@BalkanMadman
Copy link
Author

Two tests in https://github.com/quickjs-ng/quickjs/actions/runs/21075154727/job/60616560614?pr=1225 take a minute each. Is that known/expected?

35/53 quickjs-ng:regexp_ascii           OK              60.86s
36/53 quickjs-ng:regexp_utf16           OK              60.16s

@saghul
Copy link
Contributor

saghul commented Jan 16, 2026

Yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants